-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
halo2_gadgets::poseidon
: Fix loading of padding words.
#646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Any plans to merge this? |
halo2_gadgets/src/poseidon/pow5.rs
Outdated
.map(StateWord) | ||
.map(StateWord), | ||
Some(PaddedWord::Padding(padding_value)) => region | ||
.assign_advice_from_constant( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks compatibility with existing circuits, because the constants move from being assigned in the rc_b
column to being assigned in the circuit-global constant space.
Instead, we should revert the change made in 65a89f0#diff-81e1b63d3841ada245a37c0d485d03d48fb2d477a946e5135ba4602eb0e7e6f4 and manually call assign_advice
and constrain_equal
, which would avoid this problem because we can pass Value::known
into assign_advice
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, @therealyingtong and I discussed in Office Hours how to fix this in general. The core problem is that AssignedCell::copy_advice
(correctly) propagates Value::unknown
, but assign_fixed
isn't propagating the fact that it was called with Value::known
to the AssignedCell
it returns.
The simplest fix would be to always evaluate to
inside assign_fixed
, but that would result in significant proving performance regressions for existing circuits (as this bug in question meant that you just couldn't write circuits that way, i.e. it failed safe). Ideally we could use memoization to remember to
until the first time we need to evaluate it, then evaluate once, and remember the result going forward. We tried that approach and ran into both interior mutability needs, and object safety / lifetime issues related to the RegionLayouter
trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be sufficient just to make sure that the MockProver
fails in the same way as the real prover?
This test passes the MockProver, but fails to generate a real proof.
fb7047e
to
f4293c2
Compare
Force-pushed to revert 65a89f0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f4293c2. I confirmed that this does not change the Orchard circuit (as expected, because we still call assign_advice
followed by constrain_equal
with the same cell relationships).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-hoc utACK f4293c2.
Nit: the message of the second commit is "Revert commit 65a89f0", but it is not a full reversion, it reverts just the change to load_input_word
inside Pow5Chip::add_input
. It's fine, but in general we should be careful that commit messages are accurate especially for circuit changes.
Previously, a padding word was first assigned to a fixed column
using
assign_fixed()
, then copied to thestate
advice column usingcopy_advice()
. However, since the prover ignores fixed assignmentsduring synthesis, the value passed to
copy_advice
isNone
.This causes the odd-length input to pass the
MockProver
, but failin real proof generation with a
Synthesis
error.